Add custom sniffs to address spacing of operators#134
Add custom sniffs to address spacing of operators#134grongor wants to merge 1 commit intodoctrine:masterfrom grongor:add-operator-spacing-sniff
Conversation
grongor
changed the title
grongor
changed the title
grongor
changed the title
| use const T_VARIABLE; | ||
| use const T_WHITESPACE; | ||
|
|
||
| final class NegationOperatorSpacingSniff implements Sniff |
There was a problem hiding this comment.
Don't we need to add this one to ruleset.xml?
There was a problem hiding this comment.
Sniffs from the directory of the standards ruleset.xml are automatically included as far as I know, at least that's how it is with our coding standard: https://github.com/cdn77/coding-standard/tree/master/src/Cdn77CodingStandard
|
@grongor Please squash your commits, this PR looks ready to review :) |
carusogabriel
left a comment
carusogabriel
left a comment
There was a problem hiding this comment.
Thanks for the good work 👏
This is a pretty basic sniff that was missing from our standards.
alcaeus
left a comment
alcaeus
left a comment
There was a problem hiding this comment.
I agree with most of the spacing choices, except for the negation operator: there shouldn't be the option to force a space when negating literal values. For example, $b = - 3; is wrong in my opinion, as we're assigning -3 as a value, not the result of a computation. -3 is a literal and should be treated that way.
It gets a little more complicated when dealing with a variable or other expression after a negation: $a = -$b; vs. $a = - $b;. I find the latter easier to read, but the reasoning above is still valid for this case. For these expressions, I could live with either choice as long as we clarify literals as described above.
Regarding configuration, I'm not sure if the sniff should be configurable: for a library of sniffs like slevomat/coding-standard this obviously makes sense, but doctrine/coding-standard isn't designed to be flexible and configurable: it's mainly for our own internal consumption and for users that agree with this very opinionated view that we have. I see no benefit maintaining the additional tests and functionality of having the sniff configurable. However, it's not a blocker for me if other people think that the sniff should indeed be configurable.
| <?php | ||
|
|
||
| $a = $a-$b; | ||
| $a = $a - $b; |
There was a problem hiding this comment.
This may be outside the scope of this PR, but I'd expect this to be fixed to $a = $a - $b; without the double spaces. Spacing should be consistent, which this isn't.
I'll admit this may be done to allow aligning of multiple computations in a single block, but then I disagree with the idea of aligning expressions, especially when they may be complex: think of multiple computations with a variable number of operators; how do they align?
There was a problem hiding this comment.
I used slevomat/coding-standard test suit and it works by executing only the tested sniff. Therefore this can't be auto fixed, because it's out of the scope of the sniff. It's not a bug but an intended feature - because of that you can easily test unexpected side-effects of your sniff.
As for a fix of this in the general codebase, there is another sniff which does just that: https://github.com/doctrine/coding-standard/pull/134/files#diff-aa665abe4fddedfbb3ea5fc2f076ba7f
| } | ||
|
|
||
|
|
||
| yield - 1; |
There was a problem hiding this comment.
More detail in the main review comment, but this is an absolute blocker for me. The space after the - sign makes this look like a computation when we're yielding a literal value (-1). This definitely needs to be changed.
There was a problem hiding this comment.
I personally agree with you and I wouldn't configure my project in that way, but I like flexibility and I hate when I can't customize sniffs (or libs in general) to my liking ... and this particular option doesn't make the code any more complicated; it's literally just this line: $expectedSpaces = $this->requireSpace ? 1 : 0; (except for the test, but I would argue that it's trivial to "maintain" it), so I don't see a need to remove the option while it's so easy to provide it. I would, of course, keep "no space" as default.
There was a problem hiding this comment.
In this case it's not about making this configurable or not - it's about - 1 being significantly different from -1. $a = 0 -5; is incorrect since - is a subtraction operator in this context, but $a = - 5; is also incorrect because - is not a subtraction operator. Hence the issue with yield - 1;
|
This was solved via #148. |
6 participants
PR created in response to discussion at #123 (requested here #123 (comment) )
PHPCS v3.5 with custom properties replaces our current solution mentioned by @simPod, but it still has some issues, so this is the current state: cdn77/coding-standard@e67d294
The other thing that solves operator spacing is our NegationOperatorSpacingSniff which focuses only on the T_MINUS used as a unary operator. This will also be probably dropped when the squizlabs/PHP_CodeSniffer#2456 merges, squizlabs/PHP_CodeSniffer@82366db to be more precise.
I took the liberty of adjusting the composer.json and providing simple bootstrap/base test case to test the NegationOperatorSpacingSniff (the code is exactly same as the source on https://github.com/cdn77/coding-standard ) - please feel free to adjust it to your liking ;-)